Fixed array expression spreads into variadic const calls#52845
Fixed array expression spreads into variadic const calls#52845jakebailey merged 2 commits intomicrosoft:mainfrom
Conversation
|
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
| f2(1, 2, 3, 4, 5); | ||
| f2(1, 2, 3, 4, 5, 6, 7); | ||
| f2(...[1], 2, 3, 4, 5, 6); | ||
| f2(1, 2, 3, 4, 5, ...[6, 7]); |
There was a problem hiding this comment.
I changed this since the previous version became OK with this PR - spreading a fixed array expression into a call is now treated just as if positional arguments would be provided (thanks to getEffectiveCallArguments creating synthetic expressions)
| const inDestructuringPattern = isAssignmentTarget(node); | ||
| const inConstContext = isConstContext(node); | ||
| const isSpreadIntoCallOrNew = isSpreadElement(node.parent) && isCallOrNewExpression(node.parent.parent); | ||
| const inConstContext = isSpreadIntoCallOrNew || isConstContext(node); |
There was a problem hiding this comment.
initially, I wanted to adjust isConstContext - but spread arguments never have contextual types + asking for a contextual type of an argument calls getEffectiveCallArguments and we are within this call already, so it was creating an infinite loop
I was looking into other ways to check for the const context for such spread expressions but I couldn't figure out anything since I can't even check the signature (it's still resolvingSignature at this point).
|
@Andarist could you create a bug to track this as well? The problem itself seems pretty straightforward, but it would help us track bugs better. |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 4550763. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 4550763. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 4550763. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the perf test suite on this PR at 4550763. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 4550763. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 4550763. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
|
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Hey @jakebailey, the results of running the DT tests are ready. Branch only errors:Package: lodash Package: inquirer Package: zen-observable Package: inquirer/v8 |
|
I have yet to look at the DT changes, but, that VS Code change actually seems correct given the definition of IAction? |
|
That inquirer change seems unrelated. @gabritto |
|
The lodash change seems like a actual improvement; I think it's now correctly applying the tuple to the overloads and now one of the arguments is actually typed properly as though it were called with two args. The zen-observable change seems positive too. |
|
Ignoring the spurious |
|
Nice, added the |
I don't think you need to; I've seen that exact failure on another PR, so it's probably a glitch in the diff. |
|
@jakebailey Here they are:
CompilerComparison Report - main..52845
System
Hosts
Scenarios
TSServerComparison Report - main..52845
System
Hosts
Scenarios
StartupComparison Report - main..52845
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jakebailey
left a comment
There was a problem hiding this comment.
So, LGTM; these all feel like good changes. But, will need to update DT to reflect the improvement.

I noticed the issue while working on another PR here:
TS playground
fixes #53149